Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added CWLObjectType named union #220

Merged
merged 14 commits into from
Apr 10, 2024
Merged

Added CWLObjectType named union #220

merged 14 commits into from
Apr 10, 2024

Conversation

GlassOfWhiskey
Copy link
Collaborator

@GlassOfWhiskey GlassOfWhiskey commented Feb 7, 2023

This commit introduces a CWLObjectType named union to represent a valid CWL object. Using a more precise union type instead of Any allows for a smarter parsing from SALAD auto-generated parsers. In particular, CWLObjectType is used to define the type of the default directive in CWL InputParameter and WorkflowStepInput fields. Plus, a CWLInputFile type is introduced to represent a CWL jobfiles as map<string, CWLObjectType>.

This PR depends on common-workflow-language/schema_salad#672.

@GlassOfWhiskey GlassOfWhiskey force-pushed the named-unions-refactoring branch 3 times, most recently from 0327a00 to 85a0cb5 Compare February 7, 2023 13:53
@GlassOfWhiskey GlassOfWhiskey force-pushed the named-unions-refactoring branch from 9ec138c to fb45d9b Compare February 12, 2023 10:38
@GlassOfWhiskey GlassOfWhiskey marked this pull request as ready for review February 12, 2023 10:48
@GlassOfWhiskey GlassOfWhiskey force-pushed the named-unions-refactoring branch 2 times, most recently from 9a2db2e to 4347588 Compare February 12, 2023 11:02
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alas, merging this would make significant changes in the HTML rendering of CWL v1.2[.1]

InputArraySchema now lists MapSchema and UnionSchema as acceptable items ; this is not okay!

Adding CWLObjectType

image

@GlassOfWhiskey GlassOfWhiskey force-pushed the named-unions-refactoring branch from 4347588 to 7ab983b Compare February 17, 2023 15:33
@netlify
Copy link

netlify bot commented Apr 18, 2023

Deploy Preview for cwl-v1-2-dev ready!

Name Link
🔨 Latest commit 9a5e721
🔍 Latest deploy log https://app.netlify.com/sites/cwl-v1-2-dev/deploys/655e6e3aef8288000816eb4a
😎 Deploy Preview https://deploy-preview-220--cwl-v1-2-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@GlassOfWhiskey GlassOfWhiskey force-pushed the named-unions-refactoring branch 5 times, most recently from c429277 to c4d16f8 Compare April 18, 2023 10:13
@GlassOfWhiskey GlassOfWhiskey force-pushed the named-unions-refactoring branch 11 times, most recently from d9cedc6 to 045c942 Compare June 18, 2023 18:49
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks! We will need @tetron 's approval; and I still need to check the codegen & schema-salad changes

@mr-c mr-c requested a review from tetron June 19, 2023 15:04
@tetron
Copy link
Member

tetron commented Jun 23, 2023

Introducing this to metaschema_base.yml means introducing new features to CWL, as Michael noted and as I noted over here common-workflow-language/schema_salad#672 (review)

Also, copying a note of that same ticket:

From the Introduction to 1.2:

"inherited records can narrow the types of fields if those fields are re-specified with a matching jsonldPredicate."

I forgot that we did that, but that might mean there is a way to exclude MapSchema and UnionSchema from polluting the CWL 1.2 spec.

If not, I think this needs to be a 1.3 change.

@GlassOfWhiskey
Copy link
Collaborator Author

GlassOfWhiskey commented Nov 22, 2023

@mr-c could you please take a quick look to the rendered doc and the directives just to check if you agree that there is something not working as expected? Because, if yes, maybe we can defer these directives to a future PR, after we fix the overall behaviour of this part of Schema SALAD

@mr-c
Copy link
Member

mr-c commented Nov 22, 2023

I can not merge this into 1.2.1_proposed if it changes numbering for any existing documentation.

@GlassOfWhiskey
Copy link
Collaborator Author

But we said to merge it into the codegen branch if I remember correctly. Also because it requires Schema SALAD 1.3, which is not already out afaik

@GlassOfWhiskey
Copy link
Collaborator Author

In any case, probably the first PR that we should consider is common-workflow-language/schema_salad#672, as this one heavily depends on it

@mr-c
Copy link
Member

mr-c commented Nov 22, 2023

But we said to merge it into the codegen branch if I remember correctly. Also because it requires Schema SALAD 1.3, which is not already out afaik

Yep, time to make a post-1.2.1_proposed codegen branch. You are right, that I will be less protective of the numbering there :-)

@GlassOfWhiskey GlassOfWhiskey force-pushed the named-unions-refactoring branch 2 times, most recently from fae9a67 to 9a5e721 Compare November 22, 2023 21:10
@GlassOfWhiskey GlassOfWhiskey changed the base branch from 1.2.1_proposed to codegen November 22, 2023 21:11
@GlassOfWhiskey GlassOfWhiskey force-pushed the named-unions-refactoring branch 2 times, most recently from e91af6a to 8bb51c5 Compare November 22, 2023 21:16
@GlassOfWhiskey
Copy link
Collaborator Author

GlassOfWhiskey commented Nov 22, 2023

@mr-c I changed base branch to codegen (just created it). The last two questions are:

  • How do we want to handle the dependency from the last schema_salad package? At the moment the requirements.txt file points directly to the PR, but I think it should point to a release (1.3-alpha1?)
  • Do you want me to keep the updates to the render.bash file? One day they will be migrated to the CWL website main branch, bu at the moment there is no Base.yml in the CWL main branch. Do you want me to replicate the Base.yml thing also in the main branch (the 1.2.1-proposed one) without the additional features to align the logic?

mr-c and others added 13 commits April 10, 2024 15:32
also run the tests against cwltool
This commit introduces a `CWLObjectType` named union to represent a
valid CWL object. Using a more precise union type instead of `Any`
allows for a smarter parsing from SALAD auto-generated parsers.
In particular, `CWLObjectType` is used to define the type of the
`default` directive in CWL `InputParameter` and `WorkflowStepInput`
fields. Plus, a `CWLInputFile` type is introduced to represent a CWL
jobfiles as `map<string, CWLObjectType>`.
CWL parsers should not follow `format` links.
@mr-c
Copy link
Member

mr-c commented Apr 10, 2024

  • Do you want me to keep the updates to the render.bash file? One day they will be migrated to the CWL website main branch, bu at the moment there is no Base.yml in the CWL main branch. Do you want me to replicate the Base.yml thing also in the main branch (the 1.2.1-proposed one) without the additional features to align the logic?

Sure!

@mr-c mr-c force-pushed the named-unions-refactoring branch from 8bb51c5 to 92c44c7 Compare April 10, 2024 12:35
@mr-c mr-c merged commit e7bd0ae into codegen Apr 10, 2024
3 checks passed
@mr-c mr-c deleted the named-unions-refactoring branch April 10, 2024 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants